feat: Multiple RequestMirrors Filters per HTTPRoute Rule #1819

Ronnie-personal · 2023-08-23T01:55:58Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

draft coding Signed-off-by: Ronnie Personal <[email protected]>

Ronnie-personal · 2023-08-23T02:21:44Z

@arkodg
Could you please take a quick look at this initial draft code change? Am I heading in the right direction?
I will spend more time on the testing.

arkodg · 2023-08-23T22:33:35Z

internal/gatewayapi/filters.go

@@ -76,7 +76,7 @@ func (t *Translator) ProcessHTTPFilters(parentRef *RouteParentContext,
 		RuleIdx:      ruleIdx,
 		HTTPFilterIR: &HTTPFilterIR{},
 	}
-
+	filterIdx := 0
 	for i := range filters {


why not just use i

why not just use i

Yes, I can use 'i' if I don't need to match the value with the index of Mirror list.

arkodg · 2023-08-23T22:35:17Z

internal/gatewayapi/filters.go

@@ -839,22 +842,26 @@ func (t *Translator) processRequestMirrorFilter(
 	// Only add missing mirror destinations


imo this entire logic of adding missing mirror destinations can be removed, and so you can simplify this logic

imo this entire logic of adding missing mirror destinations can be removed, and so you can simplify this logic

Are we building filterContext.Mirror from scratch and there is no duplicate Endpoints?
I definitely can refactor the code based on your assessment.

yes we dont need to consider the duplicate case, please go ahead and refactor

internal/ir/xds.go

arkodg · 2023-08-23T22:36:43Z

looks good @Ronnie-personal , added some comments, thanks for picking this one up

Signed-off-by: Ronnie Personal <[email protected]>

codecov · 2023-08-25T02:15:11Z

Codecov Report

Merging #1819 (93cc046) into main (1f08e46) will decrease coverage by 0.02%.
The diff coverage is 57.44%.

@@            Coverage Diff             @@
##             main    #1819      +/-   ##
==========================================
- Coverage   66.14%   66.13%   -0.02%     
==========================================
  Files          86       86              
  Lines       12874    12868       -6     
==========================================
- Hits         8516     8510       -6     
  Misses       3830     3830              
  Partials      528      528

Files Changed	Coverage Δ
internal/ir/zz_generated.deepcopy.go	`13.47% <0.00%> (-0.07%)`	⬇️
internal/ir/xds.go	`73.68% <40.00%> (+0.07%)`	⬆️
internal/xds/translator/route.go	`90.80% <69.23%> (+0.03%)`	⬆️
internal/xds/translator/translator.go	`79.92% <72.72%> (+0.07%)`	⬆️
internal/gatewayapi/filters.go	`79.90% <83.33%> (-0.40%)`	⬇️
internal/gatewayapi/route.go	`88.38% <100.00%> (ø)`

... and 2 files with indirect coverage changes

Signed-off-by: Ronnie Personal <[email protected]>

Ronnie-personal · 2023-08-25T02:37:27Z

/retest

Ronnie-personal · 2023-08-25T19:38:48Z

/retest

Ronnie-personal · 2023-08-25T19:47:13Z

Can anyone help me to understand this pipeline error at https://github.com/envoyproxy/gateway/actions/runs/5971186116/job/16224598674?pr=1819#step:6:140?

I tested 'make -k lint' locally, I don't see the error.

arkodg · 2023-08-28T20:36:14Z

here is the error

./internal/xds/translator/testdata/in/xds-ir/http-route-mirror.yaml
[127](https://github.com/envoyproxy/gateway/actions/runs/5971186116/job/16224598674?pr=1819#step:6:128)
  Error: 17:7 [indentation] wrong indentation: expected 4 but found 6

Ronnie-personal · 2023-08-28T23:48:46Z

here is the error

./internal/xds/translator/testdata/in/xds-ir/http-route-mirror.yaml
[127](https://github.com/envoyproxy/gateway/actions/runs/5971186116/job/16224598674?pr=1819#step:6:128)
  Error: 17:7 [indentation] wrong indentation: expected 4 but found 6

Thank you! I will fix it.

Signed-off-by: Ronnie Personal <[email protected]>

Ronnie-personal · 2023-08-31T21:30:13Z

/retest

Ronnie-personal · 2023-08-31T21:32:00Z

What's happening with the pipeline? it only executed one check.

Signed-off-by: Ronnie Personal <[email protected]>

Ronnie-personal · 2023-08-31T23:48:35Z

/retest

arkodg · 2023-09-11T18:39:08Z

internal/ir/xds.go

@@ -261,7 +261,7 @@ type HTTPRoute struct {
 	// Redirections to be returned for this route. Takes precedence over Destinations.
 	Redirect *Redirect `json:"redirect,omitempty" yaml:"redirect,omitempty"`
 	// Destination that requests to this HTTPRoute will be mirrored to
-	Mirror *RouteDestination `json:"mirror,omitempty" yaml:"mirror,omitempty"`
+	Mirror []*RouteDestination `json:"mirrors,omitempty" yaml:"mirrors,omitempty"`


Suggested change

Mirror []*RouteDestination `json:"mirrors,omitempty" yaml:"mirrors,omitempty"`

Mirrors []*RouteDestination `json:"mirrors,omitempty" yaml:"mirrors,omitempty"`

arkodg · 2023-09-11T18:40:23Z

internal/xds/translator/testdata/in/xds-ir/http-route-mirror.yaml

@@ -13,7 +13,10 @@ http:
      endpoints:
      - host: "1.2.3.4"
        port: 50000
-    mirror:
-      name: "mirror-route-dest"
+    mirrors:


lets keep this example as is, and create one more test file for multiple-mirrors ?

Sound good, will do.

arkodg · 2023-09-11T18:43:53Z

internal/gatewayapi/filters.go

 	}
+	filterContext.Mirror = append(filterContext.Mirror, newMirror)
+	// Get the index of the last mirror added


why do we need L842-L844, can't we just assign Endpoints on L839 when creating RouteDestination ?

Signed-off-by: Ronnie-personal <[email protected]>

zirain · 2023-09-16T13:42:43Z

@Ronnie-personal can you remove following line, let's check with CI?

…way into MultipleMirrors1811

Ronnie-personal · 2023-09-16T14:20:04Z

@Ronnie-personal can you remove following line, let's check with CI?

Sorry, which line do you want to remove?

zirain · 2023-09-16T14:37:03Z

should be

gateway/test/conformance/conformance_test.go

Lines 49 to 50 in 1f08e46

    
           // TODO: Remove once https://github.com/envoyproxy/gateway/issues/1811 is fixed 
        
           tests.HTTPRouteRequestMultipleMirrors.ShortName,

Ronnie-personal · 2023-09-16T14:50:50Z

should be

gateway/test/conformance/conformance_test.go

Lines 49 to 50 in 1f08e46

// TODO: Remove once https://github.com/envoyproxy/gateway/issues/1811 is fixed

tests.HTTPRouteRequestMultipleMirrors.ShortName,

Thanks, I'm working on it right now.

Do we also need to remove https://github.com/envoyproxy/gateway/blob/main/test/conformance/experimental_conformance_test.go#L96

Signed-off-by: Ronnie-personal <[email protected]>

arkodg

LGTM thanks !

initial code change

e7a1f39

draft coding Signed-off-by: Ronnie Personal <[email protected]>

Ronnie-personal force-pushed the MultipleMirrors1811 branch from b6492d0 to e7a1f39 Compare August 23, 2023 02:15

arkodg reviewed Aug 23, 2023

View reviewed changes

internal/ir/xds.go Outdated Show resolved Hide resolved

update test cases

d903853

Signed-off-by: Ronnie Personal <[email protected]>

update mirror to mirrors

8ea547d

Signed-off-by: Ronnie Personal <[email protected]>

Ronnie-personal added 2 commits August 28, 2023 20:06

update test file

c70320e

Signed-off-by: Ronnie Personal <[email protected]>

fix conformance test

c1b816c

Signed-off-by: Ronnie Personal <[email protected]>

Ronnie-personal added 2 commits August 31, 2023 18:35

merge from main

219949f

Signed-off-by: Ronnie Personal <[email protected]>

update gatewayapi testdata

ba0a590

Signed-off-by: Ronnie Personal <[email protected]>

Ronnie-personal marked this pull request as ready for review September 1, 2023 00:24

Ronnie-personal requested a review from a team as a code owner September 1, 2023 00:24

arkodg reviewed Sep 11, 2023

View reviewed changes

Ronnie-personal marked this pull request as draft September 13, 2023 15:23

arkodg mentioned this pull request Sep 15, 2023

chore: bump gateway api to v0.8.1 #1891

Merged

address review comments

94e9217

Signed-off-by: Ronnie-personal <[email protected]>

Ronnie-personal added 3 commits September 15, 2023 23:49

update mirror to mirrors

be5748d

Signed-off-by: Ronnie-personal <[email protected]>

Merge branch 'main' into MultipleMirrors1811

d0ff46d

Mirror to Mirrors

17eff36

Signed-off-by: Ronnie-personal <[email protected]>

Ronnie-personal marked this pull request as ready for review September 16, 2023 13:06

Ronnie-personal and others added 2 commits September 16, 2023 09:20

Merge branch 'main' into MultipleMirrors1811

e86e473

Merge branch 'main' into MultipleMirrors1811

1b4c8a2

Merge branch 'MultipleMirrors1811' of github.com:Ronnie-personal/gate…

cb3254a

…way into MultipleMirrors1811

add conformance test back

93cc046

Signed-off-by: Ronnie-personal <[email protected]>

arkodg approved these changes Sep 16, 2023

View reviewed changes

zirain approved these changes Sep 17, 2023

View reviewed changes

zirain merged commit 14a395a into envoyproxy:main Sep 17, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Multiple RequestMirrors Filters per HTTPRoute Rule #1819

feat: Multiple RequestMirrors Filters per HTTPRoute Rule #1819

Ronnie-personal commented Aug 23, 2023

Ronnie-personal commented Aug 23, 2023

arkodg Aug 23, 2023

Ronnie-personal Aug 23, 2023

arkodg Aug 23, 2023

Ronnie-personal Aug 23, 2023

arkodg Aug 24, 2023

arkodg commented Aug 23, 2023

codecov bot commented Aug 25, 2023 •

edited

Loading

Ronnie-personal commented Aug 25, 2023

Ronnie-personal commented Aug 25, 2023

Ronnie-personal commented Aug 25, 2023

arkodg commented Aug 28, 2023

Ronnie-personal commented Aug 28, 2023

Ronnie-personal commented Aug 31, 2023

Ronnie-personal commented Aug 31, 2023

Ronnie-personal commented Aug 31, 2023

arkodg Sep 11, 2023

arkodg Sep 11, 2023

Ronnie-personal Sep 12, 2023

arkodg Sep 11, 2023

zirain commented Sep 16, 2023

Ronnie-personal commented Sep 16, 2023

zirain commented Sep 16, 2023

Ronnie-personal commented Sep 16, 2023

arkodg left a comment

		@@ -839,22 +842,26 @@ func (t *Translator) processRequestMirrorFilter(
		// Only add missing mirror destinations

	Mirror []*RouteDestination `json:"mirrors,omitempty" yaml:"mirrors,omitempty"`
	Mirrors []*RouteDestination `json:"mirrors,omitempty" yaml:"mirrors,omitempty"`

feat: Multiple RequestMirrors Filters per HTTPRoute Rule #1819

feat: Multiple RequestMirrors Filters per HTTPRoute Rule #1819

Conversation

Ronnie-personal commented Aug 23, 2023

Ronnie-personal commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg commented Aug 23, 2023

codecov bot commented Aug 25, 2023 • edited Loading

Codecov Report

Ronnie-personal commented Aug 25, 2023

Ronnie-personal commented Aug 25, 2023

Ronnie-personal commented Aug 25, 2023

arkodg commented Aug 28, 2023

Ronnie-personal commented Aug 28, 2023

Ronnie-personal commented Aug 31, 2023

Ronnie-personal commented Aug 31, 2023

Ronnie-personal commented Aug 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zirain commented Sep 16, 2023

Ronnie-personal commented Sep 16, 2023

zirain commented Sep 16, 2023

Ronnie-personal commented Sep 16, 2023

arkodg left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 25, 2023 •

edited

Loading